Pull request overview

This PR tightens correctness and security of declarative workflow execution, focusing on (1) preventing TOCTOU-style state mutation during approval windows for function-tool invocations, and (2) improving MCP client caching isolation so different auth/header/connection contexts don’t share the same cached client.

Changes:

• Capture and persist an approval-time snapshot for InvokeFunctionToolExecutor, use it on resume, and clear it after completion.
• Update DefaultMcpToolHandler client caching to discriminate by URL/label/connection/headers (with a deterministic SHA-256 header-set hash) and harden default HttpClient handler behavior.
• Add targeted unit tests covering approval snapshot behavior and MCP cache-key/hash discrimination.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Automated Code Review

Reviewers: 5 | Confidence: 89% | Result: All clear

Reviewed: Correctness, Security Reliability, Test Coverage, Failure Modes, Design Approach

Automated review by peibekwe's agents

Automated Code Review

Reviewers: 5 | Confidence: 87%

✓ Correctness

This PR makes two solid bug fixes: (1) the MCP client cache key is expanded from a simple normalized URL string to a 4-tuple including server label, connection name, and a SHA256-based headers hash, fixing isolation bugs when the same URL is used with different credentials/connections; (2) an approval snapshot mechanism prevents state mutation during the approval window from changing which function/arguments are actually invoked. Both changes are well-tested and correctly implemented. The previously-reported issue (passing untrimmed URL to CreateClientAsync) has been addressed. No blocking correctness issues found.

✓ Security Reliability

The PR introduces two solid security improvements: (1) approval snapshot mechanism preventing TOCTOU attacks on function invocation, and (2) proper cache key discrimination for MCP clients. One medium-severity hash collision vulnerability exists in ComputeHeadersHash where the key:value\n serialization format is ambiguous when header values contain newlines, potentially allowing different credential sets to map to the same cached MCP client.

✓ Test Coverage

The PR adds strong test coverage for the new approval-snapshot security feature and the MCP cache-key improvements. However, there is one new production code path — the backward-compatibility fallback in InvokeRegisteredFunctionAsync when _approvalSnapshot is null — that has no corresponding test.

✓ Failure Modes

The PR fixes two distinct issues: (1) MCP client cache key isolation is improved by using a 4-tuple (URL, label, connection, headers-hash) with SHA256 for deterministic hashing, and cookie isolation via UseCookies=false; (2) a TOCTOU security fix for InvokeFunctionToolExecutor captures an approval snapshot at request time and uses it on resume, surviving checkpoint/restore cycles. Both changes are well-structured with proper error handling, fallback paths for legacy checkpoints, and cleanup after completion. No concrete failure modes identified that meet the evidence threshold.

✗ Design Approach

I found one blocking design issue in the new function-approval snapshot flow. The restored snapshot arguments are passed straight into AIFunctionArguments, so structured arguments can change shape after a checkpoint/restore cycle: the durable state layer round-trips them through JSON, and the repo already has a normalization step for this exact problem in the normal function-call path.

Flagged Issues

• InvokeFunctionToolExecutor replays restored approval arguments directly into AIFunctionArguments without calling NormalizePortableValues() (line 332). After a durable checkpoint/restore, Dictionary<string, object?> values deserialize as JsonElement, whereas the existing function-invocation path in WorkflowRunner.cs:346 already normalizes these. Approved calls with nested arguments will silently invoke the function with different runtime types after restore.

Automated review by peibekwe's agents